Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for process launch env. vars. #127

Merged
merged 3 commits into from
Mar 8, 2021

Conversation

sambhav
Copy link
Contributor

@sambhav sambhav commented Mar 3, 2021

Signed-off-by: Sambhav Kothari skothari44@bloomberg.net

Summary

Adds support for process specific launch env vars.

Fixes #63

Use Cases

To allow setting process specific env vars on launch

I see this was previously worked upon in #70

I think it might be simpler to just expose it in the layer instead of in the process struct. This goes well with the rest of env vars that are currently housed in layers and also if someone wants to create a separate layer just for their process - that should work fine. It also allows buildpacks to supply launch env. vars for processes not defined by them.

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.

Signed-off-by: Sambhav Kothari <skothari44@bloomberg.net>
@sophiewigmore
Copy link
Member

sophiewigmore commented Mar 5, 2021

Hi @samj1912 thank you for the contribution!
I have a question about this implementation. Maybe I'm missing something here - processes can exist independent of a layer. So if ProcessLaunchEnv is only exposed in the Layer struct, how will we be able to set up process specific environment variables for processes that aren't associated with a layer?

IIRC, part of the reason #70 was closed out (we should've documented this better on the PR) was because we didn't want to force layer creation for processes just to allow for environment variables to work.

@sambhav
Copy link
Contributor Author

sambhav commented Mar 5, 2021

@sophiewigmore IIUC the spec doesn't allow for you to set process specific environment variables without creating a layer. This PR was supposed to simply match packit functionality with the spec. I know this is not ideal but I think there is very little we can do about it right now. This also goes in line with the other env variables that can be set in the layer struct.

I think we should create an upstream issue to allow users to specify env vars for a process without creating an additional layer, but I think meanwhile we should merge this so that atleast we are providing appropriate bindings for the spec.

Also IIUC this PR might be different from the previous PR which explicitly added an additional layer for storing process env vars. Instead, this allows you to use an existing layer or create a new one based on your needs.

Lmk what you think :)

Copy link
Member

@ryanmoran ryanmoran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a reasonable implementation of the current spec. The spec's implementation is unfortunate, but we should still at least support what is currently specified. Thank you @samj1912 for taking the time to implement this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend packit.Process to include process-specific environment variables
3 participants